c_variadic: Add future-incompatibility warning for ... arguments without a pattern outside of extern blocks
#143619
Conversation
|
|
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
This PR changes a file inside |
|
@bors2 try for the crater |
|
Unknown command "tryfor". |
|
|
wild @bors2 try |
`c_variadic`: Make `fn f(...) {}` error like `fn f(u32) {}` outside of `extern` blocks
This PR makes unnamed `...` parameters (such as the one in `unsafe extern "C" fn f(...) {}`) a parse error to be consistent with `unsafe extern "C" fn f(u32) {}`: this is a source of confusion for programmers coming from C, where the `...` parameter is never named and instead calling `va_start` is required; disallowing unnamed `...` parameters also improves the overall consistency of the Rust language by matching the treatment of other unnamed parameters. Unnamed `...` parameters in `extern` blocks (such as `unsafe extern "C" { fn f(...); }`) continue to compile after this PR, as they are already stable and heavily used (and don't cause the mentioned confusion as they are just being used in function declarations).
As all the syntax gating for `c_variadic` has been done post-expansion, this is technically a breaking change. In particular, code like this has compiled on stable since Rust 1.35.0:
```rust
#[cfg(any())] // Equivalent to the more recent #[cfg(false)]
unsafe extern "C" fn bar(_: u32, ...) {}
```
Since this is more or less a stability hole and is unlikely to be used in practice, I think it would be ok to break this, but this will definitely require both a crater check run and a lang FCP.
Tracking issue: #44930
cc `@folkertdev` `@workingjubilee`
r? `@joshtriplett`
|
So, should we start that crater run here? |
|
@compiler-errors could you |
|
👍 @craterbot check |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚨 Report generation of 🆘 If you need assistance dealing with this failure, please ask in t-infra on Zulip |
|
not sure I have the permissions for this, but let's attempt @craterbot retry-report |
|
🛠️ Generation of the report for ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🚨 Report generation of 🆘 If you need assistance dealing with this failure, please ask in t-infra on Zulip |
|
let's try again with rust-lang/crater#787 merged @craterbot retry-report |
|
🛠️ Generation of the report for ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
removed
the
S-waiting-on-crater
|
@rfcbot resolved |
traviscross
added
I-lang-radar
rfcbot
added
finished-final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
beetrees
force-pushed
the
varargs-named
branch
from
e8e12c0 to
652ab4b
Compare
This comment has been minimized.
This comment has been minimized.
beetrees
force-pushed
the
varargs-named
branch
from
652ab4b to
3813aeb
Compare
|
☔ The latest upstream changes (presumably #145773) made this pull request unmergeable. Please resolve the merge conflicts. |
beetrees
force-pushed
the
varargs-named
branch
from
3813aeb to
32f8c9a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
beetrees
force-pushed
the
varargs-named
branch
from
32f8c9a to
ab36e0a
Compare
|
Now that FCP is complete this needs review from T-compiler r? T-compiler |
rustbot
added
the
T-compiler
|
☔ The latest upstream changes (presumably #146125) made this pull request unmergeable. Please resolve the merge conflicts. |
beetrees
force-pushed
the
varargs-named
branch
from
ab36e0a to
403dc8b
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #146185) made this pull request unmergeable. Please resolve the merge conflicts. |
…without a pattern outside of `extern` blocks
beetrees
force-pushed
the
varargs-named
branch
from
403dc8b to
5dcbd08
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This PR makes
...arguments without a pattern in non-foreign functions (such as the argument inunsafe extern "C" fn f(...) {}) a future-compatibility warning; making this error would be consistent with howunsafe extern "C" fn f(u32) {}is handled. Allowing...arguments without a pattern in non-foreign functions is a source of confusion for programmers coming from C, where the...parameter is never named and instead callingva_startis required; disallowing...arguments without a pattern also improves the overall consistency of the Rust language by matching the treatment of other arguments without patterns....arguments without a pattern inexternblocks (such asunsafe extern "C" { fn f(...); }) continue to compile without warnings after this PR, as they are already stable and heavily used (and don't cause the mentioned confusion as they are just being used in function declarations).As all the syntax gating for
c_variadichas been done post-expansion, this is technically a breaking change. In particular, code like this has compiled on stable since Rust 1.35.0:Since this is more or less a stability hole and a Crater run shows only the
binrwcrate is using this, I think it would be ok to break this. This will require a lang FCP.The idea of rejecting
...pre-expansion was first raised here #143546 (comment).Tracking issue: #44930
cc @folkertdev @workingjubilee
r? @joshtriplett